perf: Replace HashSet with generational vec#261
Conversation
|
@cfallin I was told you might want to have a look at this. |
|
This is a small and (maybe too) focused improvement in the direction that is also discussed here: #87 |
|
@michael-weigelt thanks for this. It's an interesting optimization. Could you build a Wasmtime engine ( Those speedups are frankly fantastic if real but I'm also more than a little skeptical about purported 2-3x speedups; I've done a lot of profiling of regalloc2 and, at least last time I did this (a few years ago now), didn't see major hotspots outside of the expected ones (e.g. the main allocation maps). That said, I welcome benchmark results that say otherwise if reliable! Data-structure-wise, I was initially skeptical in reading the patch about the dense representation but I see that it's a reused set so we will only allocate one per function compilation -- so it's O(n) time and space cost overall, which is acceptable. It's still worse than the O(k) cost for k << n (very few conflicts), so I do want a careful evaluation over the suite. Side-note: in BA we now have the AI tools policy, and while you are welcome to use tools such as Claude privately (many of us do for various purposes), you are fully and solely responsible for interactions and discussions here, so instead of "Claude said [thing you're unsure about]", please speak only about things you've personally understood/vetted and don't relay raw agent thoughts or output. In this case that vetting includes running the benchmarks, and carefully reasoning about the algorithmic tradeoff as humans ourselves. Thanks! |
|
Also: I would want to see that Wasmtime+Cranelift pass all tests wth the modified regalloc2, just to make sure we're not missing something wrt changed output (which could also explain huge compile-time wins if e.g. we suddenly treat conflicts differently and allocate less optimally or whatever). The algorithm looks correct but let's verify. |
|
Thanks for the pointers, I'll run those benchmarks. I don't expect the speedups of the general cases to be this impressive, but if they are at least not worse, I am happy.
Understood and agreed. The whole point of that sentence was: I want to vet this. For the record, because I am a new contributor here: The only AI contributions in this PR are the brainstorming on how to address the problem and code. I would never relay slop to a human or let it talk/write prose for me. |
I ran them all ( I ran
But it's quite variable by how much they differ. I am interested to hear your thoughts about this, @cfallin. I don't know what operations points are most important for this library, in particular idk if you care about the noopt case. Furthermore, the PR is motivated by a uncommon Wasm module, so I do realize I have the double disadvantage of a special operation point and a special input. All that is to say: I'd understand if you reject this PR. But in its favour at least I'd say it does not look clearly worse on average, and massively better in the very special case. |
|
Thanks, @michael-weigelt . I think the best way forward here is to be objective with the benefit of data: if the PR causes more compilation slowdowns than speedups, it's not worth it overall (for a general-purpose compiler). I guess I'd that this is not too surprising when the initial evaluation is a Wasm of a "special" shape -- undoubtedly one could specialize the compiler for certain corners of the parameter space and get better performance in those cases, but we unfortunately have to build a compiler that works well for most typical inputs. Thanks nevertheless for your efforts here! |
When compiling certain types of functions (e.g. with many locals), a lot of time is spent in
try_to_allocate_bundle_to_reg, in particular onHashSet::insert. This data structure is only used for deduplicating and the cost can be quite high (hashing, allocations, bad cache properties).Replacing it with a generational vector gives significant improvements for my ad-hoc benchmark (deliberately compiled with no optimizations, because my use-case currently demands it):
Compiling a Wasm function with X locals before and after this patch:
The Sightglass benchmarks are mixed, see the comment below.